Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Test : Create unit tests for User Info #79

Closed
wants to merge 6 commits into from
Closed

Test : Create unit tests for User Info #79

wants to merge 6 commits into from

Conversation

decon-harsh
Copy link
Member

Description

Created Test database test_osp
Added few unit tests for User Info Get & Post, Login & Register routes.

Fixes #78

Type of Change:

  • Code
  • Quality Assurance
  • Documentation

Code/Quality Assurance Only

  • This change requires a documentation update (software upgrade on readme file)
  • New feature (non-breaking change which adds functionality pre-approved by mentors)

How Has This Been Tested?

Automated tests

Screenshot from 2021-02-15 03-02-55

Checklist:

  • My PR follows the style guidelines of this project
  • I have performed a self-review of my own code or materials
  • I have commented on my code or provided relevant documentation, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation

Code/Quality Assurance Only

  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@decon-harsh
Copy link
Member Author

decon-harsh commented Feb 14, 2021 •
edited
Loading

@isabelcosta Please have a look on this! Thank you for the resources! Made the work really easy

@isabelcosta
Copy link
Member

@decon-harsh you went a little above and beyond 😅 I was mentioning the GET user info, but you did tests for the user info whole API. which is fine :) 💪🏾 I will review as soon as I can. Meanwhile, please ask for reviews from other contributors on Zulip.

@isabelcosta isabelcosta added the Status: Needs Review PR needs an additional review or a maintainer's review. label Feb 14, 2021
@decon-harsh
Copy link
Member Author

decon-harsh commented Feb 14, 2021 •
edited
Loading

So sorry, i merged two seperate issues, of two different repos. I wrote Login and Register , Is it fine or Should I remove them?

@isabelcosta

@isabelcosta
Copy link
Member

@decon-harsh it's fine I think unless that is work from another contributor's issue. If it is, then please remove it, so the other contributor can do it. Is it #63 ?

@decon-harsh
Copy link
Member Author

Yes it is , some part for sure. Removing those. It won't be repeated again!

@codesankalp
Copy link
Member

codesankalp commented Feb 14, 2021 •
edited
Loading

@decon-harsh
There is no need to create a new database for the test purpose.
Just update the permissions of osp user to create a database and then Django can create a test database every time when it runs the test and deletes it after the test.

I will review your work today 👍

@decon-harsh
Copy link
Member Author

@codesankalp I guess I did same. Please let me know if i am wrong

@decon-harsh decon-harsh changed the title Test : Create unit tests for User Info , Login & Register Test : Create unit tests for User Info Feb 14, 2021
codesankalp
codesankalp reviewed Feb 14, 2021
View reviewed changes
main/settings.py Outdated
@@ -120,6 +121,9 @@
'PASSWORD': "osp",
'HOST': "localhost", # Change to db for docker-compose
'PORT': 5432,
'TEST':{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this thing is not needed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Owh , It will create test database on it's own .
Btw what will be the name of the database created?
Will it be same "test_osp" ? And if it can create test database on it's own , then what is the need of this Test {name: ""}

Copy link
Member

@codesankalp codesankalp Feb 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the alias database name will be test_osp and the owner will be the user osp.
There is no need to specify 'TEST' in database configuration, you can remove these lines. @decon-harsh

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved

@codesankalp
Copy link
Member

@decon-harsh Can you rebase your branch so that code is formatted?

@decon-harsh
Copy link
Member Author

@codesankalp sorry i didn't get it , do you want me to pull the upstream ? Or do you want me to use any formatter??

@codesankalp
Copy link
Member

@codesankalp sorry i didn't get it , do you want me to pull the upstream ? Or do you want me to use any formatter??

Rebase your branch with the develop branch by fetching and rebasing the upstream.
After this follow the steps on README for QA-CHECKS to reformat your code @decon-harsh and push again after committing the changes.

@decon-harsh
Copy link
Member Author

decon-harsh commented Feb 20, 2021 •
edited
Loading

@codesankalp I squashed latest commit and it merged all commits. Closing this PR will open a new one? Works?

@decon-harsh decon-harsh deleted the Issue78 branch February 20, 2021 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codesankalp codesankalp codesankalp left review comments

Assignees
No one assigned
Labels
Status: Needs Review PR needs an additional review or a maintainer's review.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Create unit tests for User info API
6 participants
@decon-harsh @isabelcosta @codesankalp @Rahulm2310 @sahil9001 @rmpsc